why lose the validation of the return type?

makeAccount is no longer an async function, so I didn't think .callWhen() was appropriate. Reverted in 1471410

I believe that .callWhen is not simple validation and indeed make the function become async. @erights for confirmation, but we may need to manually validate this, or find a way to make this pattern check compatible with upgradable methods.

Isn't the problem whether an await happens in the method? If the JS code that this is guarding doesn't have the async keyword then it can't await.

How could the guard change that?

The problem is doing any promise "then" if the promise may not be resolved until a future crank. Whether that's done by an await, by a when or any other mechanism that under the hood wraps promise.then(). callWhen is such a mechanism as, from what I understand, it internally when the arguments and delays the invocation of the user function until after the arguments have settled.

since these aren't exported, I think they would read better inlined..

if you don't then please capitalize the way other interface guards are, StudlyCaps

1471410

out of scope, but what motivates this getPower abstraction? it makes it harder to review.

There's a comment here:

thanks, that makes sense. I was looking on getPower.

So that explains why a store instead of a record, and I take it the getter is because maps assume homogeneity of value types. That could be worthwhile to include as a comment.

btw, going forward s/7337/7407 #7337 (comment)

we talked about getPower and how to get rid of it, right, @0xpatrickdev ? (IOU link)
we don't need to complicate access to known state fields to provide for expansion.
we just need one reserved-for-future-use property

• refactor(vats): for state upgrade, use single field #9353

Thanks @dckc . I'll tackle this in a follow-up PR

if we're always going to have to when(watch( is there a worthwhile convenience function to make? It's fine to chain them but it ends up increasing the indentation.

maybe a helper facet could manage it? perhaps validated automatically against the watcher facet

I think we ultimately only want to return watch. when is a stop gap during the refactor so we still return a promise.

is the subzone necessary?

I'm not sure. I was following the example here:

At least for the network and ibc vats, we've followed the pattern of creating a subzone specifically for all vow-related data

Why? What happens if you don't make a subzone?

Ok this is interesting. A good API design is to not throw synchronously if you return a promise. Previously because the function was async, the call would return a rejected promise with this error. The equivalent would be to return a rejected vow, but in general, this is very cumbersome to get right all the time. I am wondering if we don't need to revive something like the promise constructor pattern and wrap all synchronous prelude into a helper.

Something along the lines of

/**
 * @template {any} T
 * @param {(...args: any[]) => Vow<Awaited<T>> | Awaited<T>} fn
 * @returns {Vow<Awaited<T>>}
 */
const asVow = fn => {
  const kit = makeVowKit();
  try {
    kit.resolver.resolve(fn());
  } catch(e) {
    kit.resolver.reject(e);
  }
  return kit.vow;
};

and then

        close() {
          return when(asVow(() => {
            /// TODO #9192 what should the behavior be here? and `onClose`?
            // - retrieve assets?
            // - revoke the port?
            const { connection } = this.state;
            if (!connection) throw Fail`connection not available`;
            return watch(E(connection).close());
          }));
        },

Thanks for highlighting this. I’ve added comments in areas where we can fail synchronously referencing this comment and the parent issue in 40290c1.

I suppose the same would also apply for something like provideICQConnection, where we can return synchronously early?

 /**
   * @param {IBCConnectionID} controllerConnectionId
   * @returns {ICQConnection | Promise<ICQConnection>}
   */
  provideICQConnection(controllerConnectionId) {
    if (this.state.icqConnections.has(controllerConnectionId)) {
      return this.state.icqConnections.get(controllerConnectionId)
        .connection;
    }
    const remoteConnAddr = makeICQChannelAddress(controllerConnectionId);
    const portAllocator = getPower(this.state.powers, 'portAllocator');
    return when(
      watch(
        // allocate a new Port for every Connection
        // TODO #9317 optimize ICQ port allocation
        E(portAllocator).allocateICQControllerPort(),
        this.facets.requestICQChannelWatcher,
        {
          remoteConnAddr,
          controllerConnectionId,
        },
      ),
    );
  },

This doesn't look like it needs to be part of the kit. Why not have ../utils/packet.js provide a singleton parseTxPacketWatchHandler that can be used by all calls? But I suppose that'd still require a prepare call accepting a zone.

Thanks, taking note of this pattern for the future. For now I've decided to keep the handler as a facet on the chainAccountKit - it doesn't seem like we'll need parseTxPacket elsewhere.

I adopted your naming suggestions in 0fc3ca1 and 438446a - but am just calling it a Watcher and not a WatcherHandler for brevity.

Instead of nesting the call of E(port.connect) within requestICQConnectionWatcher, you could instead take an approach like this:

const icqControllerPortWatcher = watch(E(portAllocator).allocateICQControllerPort(), this.facets.requestICQConnectionWatcher)
return when(watch(icqControllerPortWatcher, this.facets.portConnectWater, context))

s/icqControllerPortWatcher/icqControllerPortVow/